Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: LTI 1.3 reusable configuration #390

Conversation

kuipumu
Copy link
Contributor

@kuipumu kuipumu commented Jul 10, 2023

Description

This PR adds compatibility for LTI 1.3 external configurations to the LTI consumer XBlock. This change allow the XBlock to use values from the external LTI configuration when enabled and setup. This PR also includes a fix that adds validation to the external_config field on the LTI consumer XBlock and also improves the behavior of the JS that handles the visibility of configuration fields on the LTI consumer XBlock.

Type of Change

  • Modify urlpatterns variable on plugins/urls.py module.
  • Modify get_lti_1p3_launch_info function on api.py module.
  • Modify LtiConsumerXBlock validate_field_data method.
  • Modify LtiConsumerXBlock _get_lti_block_launch_handler method.
  • Modify LtiConsumerXBlock _get_lti_1p3_launch_url method.
  • Modify LtiConfiguration clean method.
  • Modify LtiConfiguration get_lti_advantage_ags_mode method.
  • Modify LtiConfiguration get_lti_advantage_deep_linking_enabled method.
  • Modify LtiConfiguration get_lti_advantage_deep_linking_launch_url method.
  • Modify LtiConfiguration get_lti_advantage_nrps_enabled method.
  • Modify LtiConfiguration get_lti_1p3_redirect_uris method.
  • Modify LtiConfiguration _get_lti_1p3_consumer method.
  • Modify xblock_studio_view.js.
  • Modify public_keyset_endpoint function on plugin/views.py module.
  • Modify access_token_endpoint function on plugin/views.py module.
  • Include tests for all added and modified code.

Testing

  1. Install and configure openedx-ltistore (follow the setup steps from: [BB-5559] Adds support for external LTI configurations using openedx-filters #239 and use the related PR: feat: LTI 1.3 reusable configuration open-craft/openedx-ltistore#2)
  2. Create an IMS reference tool: https://lti-ri.imsglobal.org/lti/tools
  3. Go to http://localhost:18000/admin/lti_store/externallticonfiguration/ and create a new external LTI 1.3 configuration.
  4. Fill the "LTI 1.3 Private Key", "LTI 1.3 Client ID" and "LTI 1.3 Tool Keyset URL" or "LTI 1.3 Tool Public Key" field.
  5. Create a course and add a LTI 1.3 consumer XBlock.
  6. Set the consumer XBlock to user an external configuration and set the reusable configuration ID (follow the steps on how to get the filter key from: [BB-5559] Adds support for external LTI configurations using openedx-filters #239)
  7. Copy the keyset and access token URL from the XBlock preview back to the IMS reference tool previously created.
  8. Go to the course a launch the XBlock.
  9. The LTI 1.3 launch should be valid.
  10. Check that the XBlock doesn't allow you to set the external configuration type without adding an external configuration ID.
  11. Check that the XBlock doesn't allow you to set an invalid external configuration ID format.

fix: Improve external config fields validation
fix: Improve hide/show fields toggle

Co-authored-by: Squirrel18 <daniel.quiroga@edunext.co>
Co-authored-by: alexjmpb <alexander.mendoza@edunext.co>
@openedx-webhooks
Copy link

openedx-webhooks commented Jul 10, 2023

Thanks for the pull request, @kuipumu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 10, 2023
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d3686bf) 97.94% compared to head (8087346) 97.95%.
Report is 9 commits behind head on master.

❗ Current head 8087346 differs from pull request most recent head c9a2521. Consider uploading reports for the commit c9a2521 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   97.94%   97.95%   +0.01%     
==========================================
  Files          77       77              
  Lines        6555     6686     +131     
==========================================
+ Hits         6420     6549     +129     
- Misses        135      137       +2     
Flag Coverage Δ
unittests 97.95% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lti_consumer/api.py 98.92% <100.00%> (+0.06%) ⬆️
lti_consumer/exceptions.py 100.00% <100.00%> (ø)
lti_consumer/lti_xblock.py 96.64% <100.00%> (+0.01%) ⬆️
lti_consumer/models.py 91.74% <100.00%> (-0.42%) ⬇️
lti_consumer/plugin/views.py 98.06% <100.00%> (+0.12%) ⬆️
lti_consumer/tests/unit/plugin/test_views.py 100.00% <100.00%> (ø)
lti_consumer/tests/unit/test_api.py 100.00% <100.00%> (ø)
lti_consumer/tests/unit/test_lti_xblock.py 99.27% <100.00%> (+0.02%) ⬆️
lti_consumer/tests/unit/test_models.py 100.00% <100.00%> (ø)
lti_consumer/utils.py 91.59% <100.00%> (+0.21%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 12, 2023
@mphilbrick211
Copy link

Hi @kuipumu! Is this all set for review/merge?

@Zacharis278
Copy link
Contributor

@kuipumu thank you! I'll have more opportunity to dig into these PR's more deeply next week. That said this looks really promising. I could have sworn there was a catch to adopting this implementation for 1.3 but this seems pretty straightforward.

@kuipumu
Copy link
Contributor Author

kuipumu commented Jul 12, 2023

@Zacharis278 I only know that the OpenCraft team had a plan to decouple the LTI configuration model, that will meant this implementation will need to change to adjust to those changes, right now this implementation depends on having the keyset and access token URLs being on the external app.

@mphilbrick211 Yes, this PR is ready for review/merge.

@mphilbrick211
Copy link

@Zacharis278 - are you able to merge this if it's good-to-go? I believe this repo is owned by the Cosmonauts, but I'm not sure who (aside from Chris H.) is on the team.

@Zacharis278
Copy link
Contributor

@mphilbrick211 yes I can merge once this is good to go. Just to set expectations, we've got some other priorities to juggle and I can't get to these right away. I'd also like to solicit some input from OpenCraft as well so that could take additional time before this is approved and merged.

@mphilbrick211
Copy link

Thanks, @Zacharis278! Putting updates like that in here is super helpful. I might check in again, but if you're still not ready, that's OK. Having an ETA on this, or providing another update would also help if possible.

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Aug 2, 2023
@Zacharis278
Copy link
Contributor

Zacharis278 commented Aug 7, 2023

@kuipumu

This all looks good to me and would fit with the long term plan to decouple configuration/placement from what I can tell. Maybe a little more refactoring with merging this first to move these fields back out of configuration and into placement but I don't think that should hold us up.

Brief question on the configurable deployment id. Based on our current model there's no way to have multiple deployment id's for a configuration. How would this get used? Is this just forward thinking for future development of multi-tenancy?

I'm looking through the other PR's and plan to manually test each of them. Should get through that by tomorrow

@Zacharis278
Copy link
Contributor

@tecoholic are you the right person to ping on the OpenCraft side to take a look at this? Would like to get your thoughts before merging.

@mphilbrick211 mphilbrick211 removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Aug 8, 2023
@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 9, 2023

@Zacharis278 This was included to make external configs be multi-tenancy compatible. I included the possibility to use a deployment ID set on the external config, I added a field lti_1p3_deployment_id on the external config, if the field is not set, the default deployment ID of '1' is used. According to what was already commented on the LtiAgsLineItem model, I also included the oidc_url, client_id and deployment_id fields to be able isolate each lineitem per deployment. So far I haven't found any issues on this implementation but I might be wrong. Is there any detail I'm missing regarding the use of a different deployment ID?

@Zacharis278
Copy link
Contributor

Zacharis278 commented Aug 9, 2023

Think I got it. My understanding of how this would work is right now, setting deployment id doesn't really do anything other than keeping us single tenant with a different deployment id, right? Since deployment id is on the configuration it's always a 1:1 relationship. That is to say one configuration can't be shared across multiple deployments. I'm wondering if this configurable deployment id is adding any value. Please correct me if I'm missing a good use case for it.

My concern is that this would be something we need to refactor when we do build the modeling for multi-tenancy. If existing configurations are already using custom deployment ids that could become messy to migrate.

@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 11, 2023

@Zacharis278 I think there is some confusion regarding the deployment_id, there are a couple of diagrams that more clearly represent the role of the deployment_id value on the LTI tool deployment: https://www.imsglobal.org/spec/lti/v1p3#multi-tenant-tool-registered-once-deployed-multiple-times. The role of the deployment ID is mainly on the tool deployment, the idea is that you can share the same client ID, public key on a tool while creating a distinction between different deployments with the deployment ID, this means that you would just need to deploy the tool once and reuse this same security contract, correct me if I'm wrong in any of this. I think multi-tenancy is already working by just enabling the use of a different deployment ID. There is certain details, for example with AGS, each score and lineitem should be isolated per deployment, this is addressed in here, I don't know of any other resource that's shared between tool and platform that needs to be isolated per deployment, if any that should be addressed, but I didn't notice anything else on that regards that needs to be addressed.

@tecoholic
Copy link
Contributor

@Zacharis278 Hi, I think @Agrendalath is going to review a couple of PRs related to LTI. I think he might be the better person to handle this here.

@Zacharis278
Copy link
Contributor

Zacharis278 commented Aug 11, 2023

Sorry for any confusion, I understand how it works according to the spec but my questions are specifically to the fact that Open edX does not have a way to configure multiple deployments to a single configuration yet. If we do build support for this it would likely be some model separate from the tool configuration itself (probably an 'account' concept). I want to make sure we're designing in a way that keeps that path open. Our upcoming goals around building a marketplace will eventually need this.

I'd just like to understand how deployment id on configuration as it exists here would be used right now. If there's a good use case, than yeah we should add it. I wouldn't want to get too hung up on future designs if it would block adding something useful right now.

@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 14, 2023

@Zacharis278 OK I wasn't aware about any of this, right know the use case for this is just to be able to set a deployment ID different from the default "1", so we can for example have a tool that will accept multiple XBlocks using the same client_id but with a different deployment ID so we can differentiate between different deployments of this configuration.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, thank you for this contribution, and sorry for the delay in the review. I left some comments here and in the openedx-ltistore PR.

lti_consumer/models.py Outdated Show resolved Hide resolved
Comment on lines 559 to 562
# Use the xblock values if lti_1p3_oidc_url or lti_1p3_launch_url
# is not set on the external configuration.
lti_oidc_url=block.lti_1p3_oidc_url or external.get('lti_1p3_oidc_url'),
lti_launch_url=block.lti_1p3_launch_url or external.get('lti_1p3_launch_url'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we introduce the XBlock configuration as a fallback? What about the value stored in the DB (self.lti_1p3_oidc_url)?
Having a fallback is helpful, but this behavior should be clearly documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We introduce it so course authors can override the OIDC URL, launch URL or deep linking launch URL, without the need to create a new configuration. We could add also the DB values as a fallback, I didn't wanted to introduce to much options on this fallback but also having the fallback on the DB might be useful. What do you suggest this be documented on?, I just thought about adding a code comment to describe this behavior but I agree it might be better to have it documented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu , ah, right, we use them as overrides, not fallbacks.

Should we also override other values (like client_id, rsa_key) through the LtiConfiguration? Quick context: I'm talking only about the LtiConfiguration or ExternalConfiguration condition since these values cannot be easily modified (or are not stored) within the XBlock fields.

I believe that the most discoverable solution would be adding this information directly to the help text of each XBlock/DB field that can be overridden this way. Something like "If the configuration is retrieved from an external store, this value overrides the one specified in the external configuration.", but with an indication that this is not applicable when you use the CONFIG_ON_DB option.

@Zacharis278, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an issue we run into with the override functionality is right now the configuration options are exclusive. You pick either xblock, database, or external. If you choose external it's ambiguous what the next most important configuration location would be. I'd agree making this not applicable to CONFIG_ON_DB is probably the simplest short term solution around that. That said, this would now differ from the 1.1 implementation. If we're going to add an override feature its behavior should be consistent across LTI versions.

I'm leaning towards suggesting we break override behavior out into it's own PR and instead focus on parity with the existing 1.1 solution in here. Taking time to discuss/consider the full list of fields that are potentially overridable at the block level and how we visually communicate to course teams how it would work might hold up the core behavior of this PR.

Copy link
Contributor Author

@kuipumu kuipumu Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zacharis278 I agree that we should make this PR only about making LTI 1.3 external configuration 1:1 with the LTI 1.1 implementation and move other specific features (multi-tenancy, field override) into separate PRs, I will create these PRs has soon as we merge this changes. I already removed anything related to field overrides and multi-tenancy from the PR.

lti_consumer/models.py Outdated Show resolved Hide resolved
Comment on lines 666 to 683
# OIDC URL, Client ID and Deployment ID
# This allows us to isolate the LineItem per tool deployment
oidc_url = models.CharField(
max_length=255,
blank=True,
null=True,
)
client_id = models.CharField(
max_length=255,
blank=True,
null=True,
)
deployment_id = models.CharField(
max_length=255,
blank=True,
null=True,
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change backward-compatible? Shouldn't we fill these fields in the migration?

Also, why are we adding these fields to this model? A single LtiConfiguration entity can have only one set of oidc_url, client_id and deployment_id values. The LtiAgsLineItem entity can be identified by the resource_link_id. If I understand this correctly, as long as the deployment ID is defined in the database configs (instead of being tied to, e.g., an organization or a Django Site), this relation should not change.

Using the single-tenant deployments was a deliberate architectural decision, so if we want to change it, it would be reasonable to write a new ADR for this. We should also prepare the documentation that explains how to configure it. We shouldn't add undocumented features.
It would be better to split this functionality into a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single LtiConfiguration entity can only have one set of oidc_url, client_id, and deployment_id values set on the DB and XBlock, but once we use external configurations, the oidc_url, client_id and deployment_id values on the external configuration could change, this will mean that the gradebook won't change even if the oidc_url, client_id and deployment_id values changed for the LtiConfiguration. Since each lineitem should only expose gradebook information directly tied to the tool deployment, I modified the LtiAgsLineItem model to include the oidc_url, client_id and deployment_id so we are querying the LtiAgsLineItem on LtiAgsLineItemViewset not only by the lti_configuration but also by the values currently being used by the LtiConfiguration. I might be wrong about this, so please correct me if I'm wrong.

Also, you are correct, this fields should be filled in the migration, otherwise, this change wouldn't be backward compatible if applied.

We noticed the possibility of using deployment IDs while using an external configuration, I can separate anything related to implementing the use of multi-tenancy with external configurations on a separate PR to avoid increasing the complexity of the initial implementation of external configurations for LTI 1.3. What do you think if I add an ADR on how to set external configurations for LTI 1.3 and another ADR about external configurations multi-tenancy on this other PR I will open?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, yes, please move the multi-tenancy-related things to a separate PR. Once we merge the reusable LTI 1.3 configs, catching potential regressions will be much easier.

What do you think if I add an ADR on how to set external configurations for LTI 1.3 and another ADR about external configurations multi-tenancy on this other PR I will open?.

Sounds reasonable to me. Do we have a rough timeline for this separate PR? It would be good to add the LTI 1.3 external configurations ADR before the Quince release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Agrendalath I removed anything related to multi-tenancy from this PR, I don't have a rough timeline for this PR, I would think that has soon has we end merging this feature I can create a PR for anything related to multi-tenancy and external configurations.

I will work on an ADR ASAP to include in this PR related to LTI 1.3 external configurations, the Quince release is expected to be released when?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, Quince will be released in December.

@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 31, 2023

@Agrendalath Sorry for my delay on the response, I responded a few of your comments here and in in the openedx-ltistore PR. I applied a few of your suggestions to the code, Thanks!.

fix: Remove multi-tenancy related changes
fix: Remove external configuration value overrides
@kuipumu
Copy link
Contributor Author

kuipumu commented Sep 6, 2023

@Agrendalath I responded a few of your comments here and in open-craft/openedx-ltistore#2 (comment). According to what you previously commented on the openedx-ltistore PR, I reviewed again the possibility of keep using the keyset/token views from this app instead of creating a new views on the openedx-ltistore app, I was able to do so, I removed the views from the openedx-ltistore PR and modified the keyset/token views on xblock-lti-consumer.

@Agrendalath
Copy link
Member

Thanks, @kuipumu! I will review and test both PRs tomorrow.

lti_consumer/api.py Outdated Show resolved Hide resolved
lti_consumer/api.py Outdated Show resolved Hide resolved
lti_consumer/lti_xblock.py Show resolved Hide resolved
lti_consumer/models.py Show resolved Hide resolved
lti_consumer/plugin/urls.py Outdated Show resolved Hide resolved
lti_consumer/plugin/views.py Outdated Show resolved Hide resolved
lti_consumer/static/js/xblock_studio_view.js Outdated Show resolved Hide resolved
lti_consumer/tests/unit/plugin/test_views.py Outdated Show resolved Hide resolved
lti_consumer/plugin/views.py Show resolved Hide resolved
Comment on lines 666 to 683
# OIDC URL, Client ID and Deployment ID
# This allows us to isolate the LineItem per tool deployment
oidc_url = models.CharField(
max_length=255,
blank=True,
null=True,
)
client_id = models.CharField(
max_length=255,
blank=True,
null=True,
)
deployment_id = models.CharField(
max_length=255,
blank=True,
null=True,
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, Quince will be released in December.

@Agrendalath
Copy link
Member

@kuipumu, please see this comment first, as it can affect other comments.

@kuipumu
Copy link
Contributor Author

kuipumu commented Sep 27, 2023

Hi @Agrendalath , I applied all your suggestions except for this previous comment which I just left a reply, I also responded and applied your suggestions and responded your comments on open-craft/openedx-ltistore#2

@Agrendalath
Copy link
Member

@kuipumu, thank you; I will review this and open-craft/openedx-ltistore#2 tomorrow.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, I'm having some trouble with testing it. I explained one issue in the comment. The other one is that when I set the configuration to CONFIG_EXTERNAL in the LtiConfiguration DB entry, it gets reverted back to CONFIG_ON_DB every time I load the XBlock.

I tried setting the external ID to lti_store:1 and lti_store:test-configuration (slug), and some other permutations, but none seem to be working for me.

I already enabled the following flags: lti_consumer.enable_database_config, lti_consumer.enable_external_config_filter. Did I miss anything in my setup?

lti_consumer/lti_xblock.py Show resolved Hide resolved
lti_consumer/models.py Outdated Show resolved Hide resolved
lti_consumer/plugin/views.py Show resolved Hide resolved
lti_consumer/models.py Outdated Show resolved Hide resolved
# TODO: This needs to change when the LTI 1.3 support is added to the external config_type in the future.
if consumer and self.lti_version == "lti_1p3" and self.config_type == "database":
# Get LTI launch URL from consumer if using database or external configuration type.
if consumer and self.lti_version == 'lti_1p3' and self.config_type in ('database', 'external'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something odd happens when I try to use the external configuration directly from the XBlock. When I set the Configuration Type to Reusable Configuration and provide a valid LTI Reusable Configuration ID, the XBlock break with Error: (1048, "Column 'version' cannot be null"). The relevant part of the traceback is:

File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1177, in author_view
  return self.student_view(context)
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1227, in student_view
  context.update(self._get_context_for_template())
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1730, in _get_context_for_template
  lti_consumer = self._get_lti_consumer()
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/lti_xblock.py", line 1128, in _get_lti_consumer
  return get_lti_consumer(config_id_for_block(self))
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/api.py", line 96, in config_id_for_block
  config = _get_lti_config_for_block(block)
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/api.py", line 76, in _get_lti_config_for_block
  lti_config = _get_or_create_local_lti_config(
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/api.py", line 46, in _get_or_create_local_lti_config
  lti_config.save()
File "/edx/shared-src/xblock-lti-consumer/lti_consumer/models.py", line 313, in save
  super().save(*args, **kwargs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the external ID set on the XBlock is not found, this is either because the reusable configuration feature is not properly setup or the external configuration doesn't exist. If you check this specific line you can notice that if no config is found the "version" value will return None, once this value is sent to the _get_or_create_local_lti_config function, when it tries to save the new values to the LtiConfiguration it fails since "version" value cannot be null. This could be fixed by adding an extra validation to any required field from the external config on _get_lti_config_for_block but I think it's out of the scope of this PR, an issue about this should be created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #420.

fix: Improve LtiConfiguration clean method test
@kuipumu
Copy link
Contributor Author

kuipumu commented Oct 11, 2023

@Agrendalath in response to this comment.

This issue is happening both for configurations using LTI 1.1 and LTI 1.3, It seems that once you load the studio view for the XBlock, the LTI configuration reverts back to the values set on the XBlock, I'm not sure how these values are sync between the XBlock and the model, will need to check further to address this issue, I would think this should be addressed in another PR since it's not directly related to the use of external configurations with LTI 1.3.

I followed the steps from this PR to setup the external reusable configuration: #239

Did you properly setup the OPEN_EDX_FILTERS_CONFIG setting?, you should have it setup on the LMS and CMS settings with the following:

OPEN_EDX_FILTERS_CONFIG = {
    # LTI Consumer External Configuration Store.
    "org.openedx.xblock.lti_consumer.configuration.listed.v1": {
        "fail_silently": False,
        "pipeline": [
            "lti_store.pipelines.GetLtiConfigurations",
        ],
    },
}

@kuipumu
Copy link
Contributor Author

kuipumu commented Oct 11, 2023

@Agrendalath I just responded to your previous comments and applied your suggestions, I also included an ADR on how to setup this feature for both LTI 1.1 and LTI 1.3.

docs/reusable_configuration.rst Outdated Show resolved Hide resolved
docs/reusable_configuration.rst Outdated Show resolved Hide resolved
docs/reusable_configuration.rst Outdated Show resolved Hide resolved
docs/reusable_configuration.rst Outdated Show resolved Hide resolved
docs/reusable_configuration.rst Outdated Show resolved Hide resolved
lti_consumer/models.py Show resolved Hide resolved
@kuipumu
Copy link
Contributor Author

kuipumu commented Oct 19, 2023

@Agrendalath I just applied your suggestions related to the setup instructions and fixed the sync_configurations method, I also extended the README.md of the openedx-ltistore to include the instructions on how to use the "Filter Key" on this PR open-craft/openedx-ltistore#2

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuipumu, thank you so much for your persistence!
Before we merge this, please update the version to 9.7.0 and add an entry to the CHANGELOG.

👍

  • I tested this: LTI 1.3 configurations can be reused
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation

README.rst Outdated Show resolved Hide resolved
@kuipumu
Copy link
Contributor Author

kuipumu commented Oct 23, 2023

@Agrendalath Just applied your latest suggestions, I also update the package version and added a new entry to the CHANGELOG

@Agrendalath Agrendalath merged commit f36725a into openedx:master Oct 24, 2023
5 checks passed
@openedx-webhooks
Copy link

@kuipumu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants